Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Call field and accessor extra initializer after field/accessor definition on the instance #12

Merged

Conversation

pzuraq
Copy link
Owner

@pzuraq pzuraq commented Nov 20, 2023

This PR updates the ordering of extra initializers added via the addInitializer context object method.

Prior to this change, all initializers added in this way would run on instantiation, before fields and accessors were defined. This resulted in extra initializers added to fields and accessors being able to run and modify the decorated value before it was fully initialized. The concept of "extra initializers" is that they are meant to perform any final setup that should occur for a given value, so it doesn't make sense for these to run before the value they are setting up has been defined.

Additionally, this solves an issue wherein the order of field/accessor initialized values are the reverse of method decoration values. This currently makes it difficult or impossible to write decorators that can be applied to both methods and fields that contain methods:

// It's not possible to ensure that both `a` and `b` behave the same
class Foo {
  @foo @bar @baz
  a() {
    console.log('test')
  }

  @foo @bar @baz
  b = () => {
    console.log('test')
  }
}

Allowing extra initializers to run in the same order as methods and after the fields have been defined enables them to work on both forms.

@pzuraq
Copy link
Owner Author

pzuraq commented Nov 29, 2023

There was consensus to merge this change at the plenary today, I'll do so as soon as I fix CI (with a deadline of getting that done by next week)

Copy link

@rbuckton rbuckton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes should fix the CI errors

spec.html Outdated
@@ -6880,6 +6894,8 @@ <h1>
1. Else,
1. Assert: IsPropertyKey(_fieldName_) is *true*.
1. Perform ? CreateDataPropertyOrThrow(_receiver_, _fieldName_, _initValue_).
1. For each element _initializer_ of _e_.[[ExtraInitializers]], do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. For each element _initializer_ of _e_.[[ExtraInitializers]], do
1. For each element _initializer_ of _elementRecord_.[[ExtraInitializers]], do

spec.html Outdated
@@ -6880,6 +6894,8 @@ <h1>
1. Else,
1. Assert: IsPropertyKey(_fieldName_) is *true*.
1. Perform ? CreateDataPropertyOrThrow(_receiver_, _fieldName_, _initValue_).
1. For each element _initializer_ of _e_.[[ExtraInitializers]], do
1. Perform ? Call(_initializer_, _F_).
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. Perform ? Call(_initializer_, _F_).
1. Perform ? Call(_initializer_, _receiver_).

Copy link

@rbuckton rbuckton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, this solves an issue wherein the order of field/accessor initialized values are the reverse of method decoration values. This currently makes it difficult or impossible to write decorators that can be applied to both methods and fields that contain methods: [...]

I'm not seeing how the order is affected by this PR. Can you clarify what you mean here? Is this related to the missing algorithm steps to append to [[ExtraInitializers]], or something else? Also, could you provide a code example that illustrates the difference between

 @foo @bar @baz
  a() {
    console.log('test')
  }

and

  @foo @bar @baz
  b = () => {
    console.log('test')
  }

that you are anticipating is solved? Without knowing what @foo, @bar, and @baz do, its hard to determine what the concern is.

If you can link to the issues this is intended to close, that might help. I think tc39/proposal-decorators#513 relates to the first concern stated in the OP, but I can't find additional context for the second concern related to @foo @bar @baz order.

@@ -4819,6 +4819,20 @@ <h1>The ClassElementDefinition Record Specification Type</h1>
The initializers of the field or accessor, if any.
</td>
</tr>
<tr>
<td>
[[ExtraInitializers]]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR doesn't seem to include the algorithm steps that add entries to this list, so it will always be empty.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[[ExtraInitializers]] are passed into ApplyDecoratorsToElementDefinition, which then creates a context object which adds the initializers there. The logic there is a bit funky, I think I can clean it up at some point so that decorators always use [[ExtraInitializers]] and we don't have the
_staticMethodExtraInitializers_ and _instanceMethodExtraInitializers_, those were from a bit of an earlier draft before there was much distinction between extra initializers

1. For each element _e_ of _staticElements_, do
1. If _e_ is a ClassElementDefinition Record and _e_.[[Kind]] is not ~field~, then
1. Let _result_ be Completion(ApplyDecoratorsAndDefineMethod(_F_, _e_, _staticExtraInitializers_, *true*)).
1. Let _result_ be Completion(ApplyDecoratorsAndDefineMethod(_F_, _e_, _staticMethodExtraInitializers_, *true*)).
Copy link

@JLHwung JLHwung Jan 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If _e._[[Kind]] is ~accessor~, we should pass in _e_.[[ExtraInitializers]] instead of _staticMethodExtraInitializers_.

Ditto for the _instanceMethodExtraInitializers_ in the next instance elements scan.

@pzuraq pzuraq changed the title Add Class and Class Element Decorators and accessor Keyword Call field and accessor extra initializer after field/accessor definition on the instance Feb 2, 2024
@pzuraq pzuraq force-pushed the call-field-accessor-initializers-after-definition branch from b851783 to 967241b Compare February 2, 2024 16:26
@pzuraq pzuraq merged commit 110f950 into decorators Feb 2, 2024
6 of 7 checks passed
@pzuraq pzuraq deleted the call-field-accessor-initializers-after-definition branch February 2, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants